feat: Add UI for PKCE and JWKS URL upstream OIDC options#623
feat: Add UI for PKCE and JWKS URL upstream OIDC options#623assimilat wants to merge 1 commit intonetbirdio:mainfrom
Conversation
|
|
📝 WalkthroughWalkthroughThe SSO identity provider configuration is extended to support PKCE authentication and JWKS URL configuration. Interface definitions add required Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/modules/settings/IdentityProviderModal.tsx (2)
270-298: Consider placing the PKCE toggle adjacent to (or above) the Client Secret field.PKCE directly governs whether the Client Secret is required, but the toggle currently sits two fields below the secret input (and after the JWKS URL). Moving the PKCE toggle just above or below the Client Secret would make the cause-and-effect relationship more discoverable and reduce the chance of users hitting a disabled submit state without understanding why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/settings/IdentityProviderModal.tsx` around lines 270 - 298, The PKCE toggle (FancyToggleSwitch using props value={pkce} onChange={setPkce}) is placed after the JWKS URL and should be moved next to the Client Secret input so users see the dependency; update the JSX so the FancyToggleSwitch is rendered immediately above or below the Client Secret Input block (reordering the component tree rather than changing props), ensuring helpText and label remain the same and that any layout wrappers/classes around the Client Secret/Input and FancyToggleSwitch are adjusted so they appear visually adjacent.
250-268: Update Client Secret help text to reflect PKCE behavior.When PKCE is enabled, the secret is no longer required (per the validation change at Line 118), but the help text still tells users it is "The OAuth2 client secret" / "Required when client ID is changed". Consider conditionally indicating that the field is optional when
pkceis true, so the UI matches the new validation rules.✏️ Suggested help-text adjustment
<HelpText> {isEditing ? clientIdChanged - ? "Required when client ID is changed" + ? pkce + ? "Optional when PKCE is enabled" + : "Required when client ID is changed" : "Leave empty to keep the existing secret, or enter a new one" - : "The OAuth2 client secret"} + : pkce + ? "Optional when PKCE is enabled" + : "The OAuth2 client secret"} </HelpText>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/settings/IdentityProviderModal.tsx` around lines 250 - 268, The HelpText for the Client Secret field is out of sync with the new PKCE validation: when pkce is true the secret is optional. Update the JSX that renders the HelpText (the block around Label/HelpText for clientSecret in IdentityProviderModal) to conditionally show a PKCE-aware message (e.g., indicate "Optional when PKCE is enabled" or "Not required when PKCE is enabled; leave empty to keep existing secret") based on the pkce prop/state, and preserve the existing messages that reference isEditing and clientIdChanged when pkce is false so the UI matches the validation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/interfaces/IdentityProvider.ts`:
- Around line 80-81: Update the type definitions so pkce and jwks_url are
optional on both SSOIdentityProvider and SSOIdentityProviderRequest: change the
required properties pkce and jwks_url to optional properties (allowing
undefined) in those interfaces so existing backends that omit these fields won’t
violate the type contract; adjust any related type usages if necessary to handle
undefined (e.g., use provider?.pkce ?? false and provider?.jwks_url ?? "" as the
UI already does).
---
Nitpick comments:
In `@src/modules/settings/IdentityProviderModal.tsx`:
- Around line 270-298: The PKCE toggle (FancyToggleSwitch using props
value={pkce} onChange={setPkce}) is placed after the JWKS URL and should be
moved next to the Client Secret input so users see the dependency; update the
JSX so the FancyToggleSwitch is rendered immediately above or below the Client
Secret Input block (reordering the component tree rather than changing props),
ensuring helpText and label remain the same and that any layout wrappers/classes
around the Client Secret/Input and FancyToggleSwitch are adjusted so they appear
visually adjacent.
- Around line 250-268: The HelpText for the Client Secret field is out of sync
with the new PKCE validation: when pkce is true the secret is optional. Update
the JSX that renders the HelpText (the block around Label/HelpText for
clientSecret in IdentityProviderModal) to conditionally show a PKCE-aware
message (e.g., indicate "Optional when PKCE is enabled" or "Not required when
PKCE is enabled; leave empty to keep existing secret") based on the pkce
prop/state, and preserve the existing messages that reference isEditing and
clientIdChanged when pkce is false so the UI matches the validation logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41672138-e04a-4aba-9de2-0ac087cce6c0
📒 Files selected for processing (2)
src/interfaces/IdentityProvider.tssrc/modules/settings/IdentityProviderModal.tsx
| pkce: boolean; | ||
| jwks_url: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the backend always returns pkce/jwks_url, and whether other consumers
# of SSOIdentityProvider rely on these fields being non-null.
rg -nP --type=ts -C3 '\b(pkce|jwks_url)\b'
rg -nP --type=ts -C2 'SSOIdentityProvider\b'Repository: netbirdio/dashboard
Length of output: 9840
Make pkce and jwks_url optional on both SSOIdentityProvider and SSOIdentityProviderRequest.
The UI code initializes these fields with defaults:
provider?.pkce ?? false(line 100 of IdentityProviderModal.tsx)provider?.jwks_url ?? ""(line 101 of IdentityProviderModal.tsx)
This pattern indicates the API may not return these fields in responses from older backends, creating a type contract violation. Existing code will silently receive undefined for these fields while the interfaces declare them as required string / boolean. Both interfaces need to make these optional for proper backward compatibility.
🛠️ Suggested type relaxation
export interface SSOIdentityProvider {
id: string;
type: SSOIdentityProviderType;
name: string;
issuer: string;
client_id: string;
redirect_url?: string;
- pkce: boolean;
- jwks_url: string;
+ pkce?: boolean;
+ jwks_url?: string;
}
export interface SSOIdentityProviderRequest {
type: SSOIdentityProviderType;
name: string;
issuer: string;
client_id: string;
client_secret: string;
- pkce: boolean;
- jwks_url: string;
+ pkce?: boolean;
+ jwks_url?: string;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pkce: boolean; | |
| jwks_url: string; | |
| pkce?: boolean; | |
| jwks_url?: string; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/interfaces/IdentityProvider.ts` around lines 80 - 81, Update the type
definitions so pkce and jwks_url are optional on both SSOIdentityProvider and
SSOIdentityProviderRequest: change the required properties pkce and jwks_url to
optional properties (allowing undefined) in those interfaces so existing
backends that omit these fields won’t violate the type contract; adjust any
related type usages if necessary to handle undefined (e.g., use provider?.pkce
?? false and provider?.jwks_url ?? "" as the UI already does).
2 participants
This PR adds UI toggles and inputs for configuring PKCE and JWKS URLs for upstream OIDC identity providers.
Summary by CodeRabbit
New Features